Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Configure hosts for application #1565

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Configure hosts for application #1565

wants to merge 1 commit into from
+5 −5

Conversation

brucebolt
Copy link
Member

Note: the healthcheck endpoints are requested by IP, not domain, so we need to specifically exclude them from the protection.

Trello card

AgaDufrat
AgaDufrat previously approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@AgaDufrat AgaDufrat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brucebolt
Copy link
Member Author

I've realised this allows domains like app-name.dodgygov.uk. The regex will need to be updated to app-name\..*\.gov.uk$.

@brucebolt
Configure hosts for application
34c24a6 
Note: the healthcheck endpoints are requested by IP, not domain, so we
need to specifically exclude them from the protection.
@nicholsj
Copy link

nicholsj commented Nov 5, 2024

In case it's relevant, note the additional allowed hostname for local-links-manager, because the original restriction caused issues

@AgaDufrat
Copy link
Contributor

The author discontinued work on this PR. Reverting to Draft (so it's not pick up by Seal) and adding it to our team's backlog.

It requires further work:

  • add internal hostname to allowed domains as the API is used internally in the cluster by argo workflows for deployments
  • the regex is too permissive and will need updating to /appname\..*\.gov.uk$/

@AgaDufrat AgaDufrat marked this pull request as draft November 20, 2024 11:05
@AgaDufrat AgaDufrat dismissed their stale review November 20, 2024 11:07

Further work required

@AgaDufrat AgaDufrat added the security Pull requests that address a security vulnerability label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AgaDufrat AgaDufrat AgaDufrat left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
security Pull requests that address a security vulnerability
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brucebolt @nicholsj @AgaDufrat